harden: secure extension and preset archive downloads by PascalThuet · Pull Request #3141 · github/spec-kit

PascalThuet · 2026-06-23T22:55:33Z

Originally rebased onto main at 4d3a428. The published head now includes a validated merge of current main (0117a7b). #3140 and #3671 are merged, so this PR reuses their shared URL, redirect, bounded-read, and checksum primitives instead of carrying the #3140 commit.

What

Replace path-only ZIP extraction for extensions and presets with bounded extraction that rejects traversal, symlinks, ambiguous normalized paths, file/descendant conflicts, non-portable Windows names, oversized members, oversized totals, and excessive entry counts.
Preflight ZIP32 central directories and local headers before ZipFile materializes entries, including declared/actual entry counts, a central-directory size cap, and archive-level or per-entry ZIP64 indicators.
Bound extension/preset catalog JSON, package downloads, direct URL installs, and extension-update manifest reads.
Fully validate extension-update manifests, compatibility, normalized catalog/package version equality, and install conflicts before removing the installed extension.
Complete recoverable backups before the destructive update boundary, including registered skill directories; rollback removes newly created skills and restores the original registry, commands, and skills.
Keep extension-update manifest selection consistent with extraction across slash, case, and Unicode normalization while preserving root-manifest priority over a distinct nested manifest.
Reject malformed URL types/hosts/ports and catalog-controlled output filenames that could escape the requested download directory.
Verify a catalog SHA-256 only when the entry provides one; checksum presence remains backward-compatible and optional.
Escape untrusted archive/download values in errors so terminal control sequences are not emitted.

Security and compatibility boundaries

Downloaded archives are capped at 50 MiB; extracted archives at 512 entries, 10 MiB per member, 50 MiB total, and a 4 MiB central directory.
Archive paths are capped at 255 UTF-8 bytes per component and 4096 UTF-8 bytes overall.
ZIP64 is deliberately rejected, including per-entry central/local ZIP64 markers and extra fields.
Bounded extraction accepts ZIP32 STORE and DEFLATE entries. BZIP2 and LZMA entries are rejected before ZipFile because the standard-library read path cannot enforce the requested decompressed-output bound for those methods.
SHA-256 validation is not catalog-wide integrity until catalogs publish hashes consistently.

Validation

Full suite on the published merged head (ab2a825, main at 0117a7b): 5641 passed, 172 skipped.
Pre-merge full branch suite: 5427 passed, 166 skipped.
Final targeted download, extension-update, skill rollback, and path-safety suite: 793 passed.
uvx ruff@0.15.0 check src tests: clean on the published merged head.
git diff --check: clean on the published merged head.
Bandit on changed production files: no medium/high findings; three low findings are pre-existing and outside this diff.
Dependency audit inputs are unchanged.
Independent review found no remaining P0/P1/P2 regression after the fixes.

Deliberately out of scope

Catalog-wide SHA-256 population.
Atomic publication of deterministic cache files and the pre-existing local symlink race around cache ancestors; this needs a separate, narrowly scoped follow-up.

Authorship disclosure: updated on behalf of @PascalThuet by OpenAI Codex (model: GPT-5).

Copilot

Pull request overview

This PR hardens extension/preset/catalog download and extraction paths by introducing bounded reads, optional SHA-256 verification, and safer ZIP handling, then wiring those protections into the extension/preset install flows and associated tests.

Changes:

Add _download_security helpers for strict URL validation, bounded HTTP reads, SHA-256 verification, bounded ZIP-member reads, and safe ZIP extraction (zip-slip/zip-bomb/symlink defenses).
Update extension and preset download/install flows to use bounded reads, checksum verification (when sha256 is present), and safe_extract_zip.
Update auth redirect handling and tests to reflect strict redirect enforcement and stream-like read(size) semantics.

Show a summary per file

File	Description
`src/specify_cli/_download_security.py`	New security primitives: bounded reads, ZIP extraction/member limits, checksum verification, scheme/host predicate.
`src/specify_cli/authentication/http.py`	Strict redirect validation integrated into redirect handler and exposed via `open_url(..., strict_redirects=...)`.
`src/specify_cli/authentication/azure_devops.py`	Bounded token-response reads and strict redirect protection for client-credentials POST.
`src/specify_cli/_version.py`	Latest-release metadata fetch now uses bounded reads and strict redirects.
`src/specify_cli/_github_http.py`	GitHub release metadata resolution now uses bounded reads to avoid unbounded JSON loads.
`src/specify_cli/catalogs.py`	Catalog URL validation rejects hostless URLs before scheme checks.
`src/specify_cli/extensions/__init__.py`	Extension ZIP installs use `safe_extract_zip`; catalog and ZIP downloads are bounded and optionally checksum-verified.
`src/specify_cli/extensions/_commands.py`	Extension manifest pre-read from ZIP is now bounded (`read_zip_member_limited`).
`src/specify_cli/presets/__init__.py`	Preset ZIP installs use `safe_extract_zip`; catalog and ZIP downloads are bounded and optionally checksum-verified.
`src/specify_cli/presets/_commands.py`	Shared URL predicate for download/redirect validation; preset ZIP downloads use bounded reads and strict redirects for GitHub metadata resolution.
`tests/test_download_security.py`	New tests covering bounded reads, checksum verification, safe ZIP extraction, and bounded ZIP member reads.
`tests/test_authentication.py`	Tests updated for strict redirect rejection and bounded read semantics; Azure AD token tests patch opener behavior.
`tests/test_github_http.py`	Tests updated for stream-like reads; add multi-hop GitHub redirect auth-preservation test.
`tests/http_helpers.py`	Stream-backed `read()` mocks and an autouse fixture to route `build_opener().open()` through patched `urlopen`.
`tests/test_upgrade.py`	Add regression test asserting `_fetch_latest_release_tag` reads via `read_response_limited(max_bytes=...)`.
`tests/self_upgrade_helpers.py`	Re-export the opener-routing fixture for self-upgrade test modules.
`tests/test_self_upgrade_detection.py`	Enable opener-routing fixture to keep existing urlopen patches effective.
`tests/test_self_upgrade_execution.py`	Enable opener-routing fixture to keep existing urlopen patches effective.
`tests/test_self_upgrade_verification.py`	Enable opener-routing fixture to keep existing urlopen patches effective.
`tests/test_extensions.py`	Stream-like `read(size)` mocks and one assertion update for host-before-scheme URL validation.
`tests/test_presets.py`	Stream-like `read(size)` mocks and open_url fake signature updated for `strict_redirects`.
`tests/integrations/test_integration_catalog.py`	One assertion updated for host-before-scheme URL validation.

Copilot's findings

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Files reviewed: 22/22 changed files
Comments generated: 2

mnriem · 2026-06-24T18:30:07Z

Converting to draft as #3140 has to land first

Adopt the shared download-security primitives from github#3140 across extension and preset catalog, package, direct-URL, and ZIP-install flows: - bound catalog, package, and inline manifest reads; - verify catalog SHA-256 values when present; - replace path-only extraction with bounded traversal/symlink-safe extraction; - validate malformed hosts and ports before opening download URLs; - handle normalized trailing-backslash directory entries consistently. Redirect enforcement and checksum verification remain owned by the shared helpers already on main; this commit wires them into extension and preset behavior. Assisted-by: OpenAI Codex (model: GPT-5, autonomous)

Preflight ZIP central directories before ZipFile allocates them, bound both declared and actual payload sizes, and reject ambiguous or non-portable archive paths before extraction. Keep extension update manifest selection consistent with extraction, reject unsafe catalog-derived output filenames and malformed URL types, and escape untrusted values in download errors. Add regression coverage for parser differentials, collisions, platform-specific filenames, bounded call sites, and failure ordering. Assisted-by: OpenAI Codex (model: GPT-5, autonomous)

PascalThuet · 2026-07-23T21:51:28Z

Rebased this PR onto current main after #3140 and #3671, then force-updated the branch to b52570e (54d5f0c for the rebased feature wiring, b52570e for the independent audit fixes).

The refreshed diff removes the duplicated #3140 history, retains the shared redirect/checksum behavior from main, and adds bounded ZIP32 central-directory preflight, portable path/collision validation, bounded direct/catalog/update reads, safe catalog-derived output filenames, and manifest selection aligned with extraction. The trailing-backslash directory feedback remains covered.

Validation on the published tree: full Python 3.12 suite 5291 passed, 166 skipped; focused Python 3.13 suite 915 passed; Ruff 0.15.0 and git diff --check clean; Bandit found no medium/high issue in changed production files. ZIP64 is intentionally rejected, SHA-256 applies only where catalogs provide hashes, and atomic cache publication/local symlink races remain a separate follow-up.

Posted on behalf of @PascalThuet by OpenAI Codex (model: GPT-5).

Copilot

Review details

Files reviewed: 8/8 changed files
Comments generated: 2
Review effort level: Medium

mnriem

Please address Copilot feedback. Also what about workflow, workflow steps and bundle downloads? Do they need this too?

Assisted-by: OpenAI Codex (model: GPT-5, autonomous)

Copilot

Review details

Files reviewed: 19/19 changed files
Comments generated: 1
Review effort level: Medium

mnriem · 2026-07-24T18:50:40Z

Please address Copilot feedback and fix test & lint errors

Assisted-by: OpenAI Codex (model: GPT-5, autonomous)

Copilot

Review details

Files reviewed: 20/20 changed files
Comments generated: 2
Review effort level: Medium

Assisted-by: OpenAI Codex (model: GPT-5, autonomous)

mnriem

Please address Copilot feedback

Copilot

Review details

Comments suppressed due to low confidence (2)

src/specify_cli/_download_security.py:472

Checking only equality with version 4.5 does not reliably reject ZIP64. A ZIP64 data-descriptor archive can change both “version needed” fields from 45 to 46 while retaining the 64-bit descriptor; ZipFile accepts it, and the preflight no longer sees any ZIP64 indicator after the sentinels/extra field are masked. Since supported STORE/DEFLATE ZIP32 entries require versions below 4.5, reject version-needed values at or above 45.

    if extract_version == _ZIP64_EXTRACT_VERSION:
        _raise_zip64(error_type)

src/specify_cli/extensions/_commands.py:1316

Root selection uses an exact case-sensitive string even though candidate identity is case-folded. An archive containing EXTENSION.YML at the root plus a valid repo/extension.yml therefore preflights the nested manifest, but after extraction install_from_zip() finds the root file as extension.yml on a case-insensitive filesystem and installs that different, unvalidated manifest. Select/reject root aliases using the portable key and ensure installation uses the same selected member on every filesystem.

                        root_manifest = next(
                            (
                                name
                                for name, normalized_name, _path_key
                                in manifest_candidates
                                if normalized_name == "extension.yml"
                            ),

Files reviewed: 21/21 changed files
Comments generated: 2
Review effort level: Medium

mnriem

Please address Copilot feedback and fix test & lint errors

Assisted-by: OpenAI Codex (model: GPT-5, autonomous)

Copilot

Review details

Comments suppressed due to low confidence (1)

src/specify_cli/agents.py:986

Nested Copilot aliases now create parent directories, but unregister_commands() only unlinks the agent and prompt files and does not prune parents for non-skill outputs. Removing an extension that registers group/run therefore leaves .github/agents/group/ and .github/prompts/group/ behind. Prune empty parents up to each output root so this new nested-alias path remains uninstall-clean.

        prompt_file.parent.mkdir(parents=True, exist_ok=True)

Files reviewed: 24/24 changed files
Comments generated: 0 new
Review effort level: Medium

mnriem · 2026-07-28T12:52:16Z

Thank you!

PascalThuet requested a review from mnriem as a code owner June 23, 2026 22:55

PascalThuet mentioned this pull request Jun 24, 2026

harden: bound remaining catalog reads + lock in no-unbounded-read invariant #3145

Draft

mnriem requested a review from Copilot June 24, 2026 12:50

Copilot started reviewing on behalf of mnriem June 24, 2026 12:51 View session

Copilot AI reviewed Jun 24, 2026

View reviewed changes

Comment thread src/specify_cli/_download_security.py Outdated

Comment thread src/specify_cli/_download_security.py Outdated

mnriem marked this pull request as draft June 24, 2026 18:29

PascalThuet added 2 commits July 23, 2026 23:47

PascalThuet force-pushed the split/extension-preset-downloads branch from 99773ba to b52570e Compare July 23, 2026 21:49

PascalThuet changed the title ~~harden: safe ZIP extraction, checksums, and bounded reads for extension/preset downloads~~ harden: secure extension and preset archive downloads Jul 23, 2026

PascalThuet marked this pull request as ready for review July 23, 2026 21:50

mnriem requested a review from Copilot July 24, 2026 12:36

Copilot started reviewing on behalf of mnriem July 24, 2026 12:36 View session

Copilot AI reviewed Jul 24, 2026

View reviewed changes

Comment thread src/specify_cli/extensions/_commands.py

Comment thread src/specify_cli/_download_security.py Outdated

mnriem requested changes Jul 24, 2026

View reviewed changes

harden: address download security review feedback

db5bda4

Assisted-by: OpenAI Codex (model: GPT-5, autonomous)

mnriem requested a review from Copilot July 24, 2026 17:53

Copilot started reviewing on behalf of mnriem July 24, 2026 17:54 View session

Copilot AI reviewed Jul 24, 2026

View reviewed changes

Comment thread src/specify_cli/_download_security.py Outdated

harden: close ZIP preflight review gaps

12d1435

Assisted-by: OpenAI Codex (model: GPT-5, autonomous)

PascalThuet requested a review from mnriem July 26, 2026 22:40

mnriem requested a review from Copilot July 27, 2026 12:12

Copilot started reviewing on behalf of mnriem July 27, 2026 12:12 View session

Copilot AI reviewed Jul 27, 2026

View reviewed changes

Comment thread src/specify_cli/extensions/_commands.py

Comment thread src/specify_cli/extensions/_commands.py

fix: harden extension update preflight and rollback

694ea40

Assisted-by: OpenAI Codex (model: GPT-5, autonomous)

mnriem requested changes Jul 27, 2026

View reviewed changes

PascalThuet requested a review from mnriem July 27, 2026 13:48

mnriem requested a review from Copilot July 27, 2026 15:19

Copilot started reviewing on behalf of mnriem July 27, 2026 15:19 View session

Copilot AI reviewed Jul 27, 2026

View reviewed changes

Comment thread src/specify_cli/extensions/_commands.py

Comment thread src/specify_cli/extensions/__init__.py Outdated

mnriem requested changes Jul 27, 2026

View reviewed changes

PascalThuet added 2 commits July 27, 2026 23:40

fix: harden extension update rollback

a88bf00

Assisted-by: OpenAI Codex (model: GPT-5, autonomous)

merge: sync latest main into PR github#3141

ab2a825

Assisted-by: OpenAI Codex (model: GPT-5, autonomous)

lilisalinas936-design mentioned this pull request Jul 27, 2026

## toro lilisalinas936-design/shiny-octo-vurnerabilidad#1

Open

mnriem requested a review from Copilot July 28, 2026 12:16

Copilot started reviewing on behalf of mnriem July 28, 2026 12:16 View session

Copilot AI reviewed Jul 28, 2026

View reviewed changes

mnriem self-requested a review July 28, 2026 12:51

mnriem approved these changes Jul 28, 2026

View reviewed changes

mnriem merged commit 118062e into github:main Jul 28, 2026
14 checks passed

jawwad-ali mentioned this pull request Jul 28, 2026

fix(bundle): wrap local .zip errors in BundlerError instead of raw crash #3713

Closed

Uh oh!

Conversation

PascalThuet commented Jun 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What

Security and compatibility boundaries

Validation

Deliberately out of scope

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Copilot's findings

Uh oh!

Uh oh!

Uh oh!

mnriem commented Jun 24, 2026

Uh oh!

PascalThuet commented Jul 23, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Review details

Uh oh!

Uh oh!

Uh oh!

mnriem left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Review details

Uh oh!

Uh oh!

mnriem commented Jul 24, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Review details

Uh oh!

Uh oh!

Uh oh!

mnriem left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Review details

Uh oh!

Uh oh!

Uh oh!

mnriem left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Review details

Uh oh!

Uh oh!

mnriem commented Jul 28, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

PascalThuet commented Jun 23, 2026 •

edited

Loading